Add nixpkgs_cc_configure() #40
Conversation
mboes
force-pushed
the
cc-configure-with-nix
branch
from
45386ad to
2582aa2
Compare
|
This is #16 reopened. |
There was a problem hiding this comment.
Looks good in principle, a few comments.
There’s the question whether we want to support both repository and repositories indefinitely. We now have two rules which allow both, plus two different ways to check for consistency (see comment below). I’m going to open a separate issue for that.
@mboes if you don’t have time, I can implement my suggested changes.
Meta comment: Does this mean we are going to need to implement wrappers like this for each language Google supports officially?
| use. | ||
| """ | ||
| if repository and repositories or not repository and not repositories: | ||
| fail("Specify one of repository or repositories (but not both).") |
There was a problem hiding this comment.
We have this logic in both nixpkgs_package and here. Actually, nixpkgs_package silently overwrites repository if repositories has a "nixpkgs" field, which is a bug. Should be factored out.
nixpkgs/nixpkgs.bzl
Outdated
| tool: repository_ctx.path(Label("@nixpkgs_cc_toolchain//:bin/" + tool)) | ||
| for tool in res.stdout.splitlines() | ||
| } | ||
| cc_autoconf_impl(repository_ctx, overriden_tools = overriden_tools) |
There was a problem hiding this comment.
elif cpu_value == "x64_windows":
# TODO(ibiryukov): overriden_tools are only supported in configure_unix_toolchain.
# We might want to add that to Windows too(at least for msys toolchain).
configure_windows_toolchain(repository_ctx)
https://github.com/bazelbuild/bazel/blob/master/tools/cpp/cc_configure.bzl#L48
Something to keep in mind.
nixpkgs/nixpkgs.bzl
Outdated
| tool: repository_ctx.path(Label("@nixpkgs_cc_toolchain//:bin/" + tool)) | ||
| for tool in res.stdout.splitlines() | ||
| } | ||
| cc_autoconf_impl(repository_ctx, overriden_tools = overriden_tools) |
There was a problem hiding this comment.
The rule uses repository_ctx.which in unix_cc_configure.bzl to resolve paths passed through overridden_tools, and it’s not clear from the documentation what that means for the absolute paths we pass it: bazelbuild/bazel#6660 (I’m too lazy to look into the Java source right now, but I assume it returns absolute paths unmodified).
I think we should. Setting only |
This refactor has two simple goals: * Simplify control flow: callers no longer need to `if-then-else` themselves, which becomes cumbersome when you have a sequence of calls to `repository_ctx.execute()`. * Improve the output on failure (by listing the failing command).
mboes
force-pushed
the
cc-configure-with-nix
branch
2 times, most recently
from
167e095 to
27b176f
Compare
|
@Profpatsch PTAL |
This macro calls cc_autoconf_impl like before, but ahead of then also defines a nixpkgs_package with all the tools we want for the CC toolchain.
mboes
force-pushed
the
cc-configure-with-nix
branch
from
27b176f to
200bff4
Compare
Yes. |
A call to this macro in your WORKSPACE file is a simple one-liner. It
declares a nixpkgs_package with all the tools we need for a CC
toolchain, and then gets Bazel to use them. Making sure the Bazel uses
the C compiler from Nixpkgs rather than whatever it autodetects on the
PATH improves build hermeticity, and side steps a whole host of issues
we've been facing on rules_haskell due to system GCC wanting to link
against the system GLIBC while Nixpkgs GCC wants another one.